Adds inactive state to ActionList items #3913

mperrotti · 2023-11-06T22:17:29Z

Relates to https://github.com/github/primer/issues/2677

Screenshots

InactiveExample-InactiveItem.mp4

Changelog

Supports inactive ActionList items by letting users pass the required message to the inactiveText prop

New

inactiveText prop on ActionList.Item

Changed

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Check out the new stories for ActionList, ActionMenu, and NavList that demonstrate inactive list items

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
[?] Changes are SSR compatible
- Not sure. I thought none of our ActionList stuff was SSR compatible due to our use of slots
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
Integration tests pass at github/github (Learn more about how to run integration tests)

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

makes unavailable message consistent fixes LinkItem styling

changeset-bot · 2023-11-06T22:17:33Z

🦋 Changeset detected

Latest commit: 9ef7b72

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-11-06T22:17:48Z

Uh oh! @mperrotti, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

github-actions · 2023-11-06T22:20:07Z

size-limit report 📦

Path	Size
dist/browser.esm.js	105.42 KB (+0.23% 🔺)
dist/browser.umd.js	105.99 KB (+0.25% 🔺)

mperrotti · 2023-11-06T22:20:48Z

src/ActionList/Item.tsx

    )

    const keyPressHandler = React.useCallback(
      (event: React.KeyboardEvent<HTMLLIElement>) => {
-        if (disabled) return
+        if (disabled || inactive) return


I'm not calling any handlers passed to onSelect for inactive items. This would also make it impossible to render a dialog when the user clicks the item. I think this is a reasonable limitation to begin with, and we can always update the implementation to support click/selection handlers so users can show a Dialog if we find we need to.

Do you agree that it's safe to prohibit onSelect on inactive items for now?

Here's a draft PR to update the interface guidelines: primer/design#654

I'll close it if we think we do need to support click/selection behavior on inactive items.
I'll merge it if we don't think we need to support click/selection behavior on inactive items.

mperrotti · 2023-11-06T22:21:19Z

src/ActionList/Item.tsx

 import Box, {BoxProps} from '../Box'
+import {Tooltip} from '../drafts'


I'm using the new, accessible Tooltip. Is this safe to use even though it's still in the experimental phase?

cc @broccolinisoup

In terms of using the new draft Tooltip, I think it is safe. It is unlikely to have API changes but still possible since it is in the experimental stage. The only API surface that is exposed in Actionlist API I see is the type of the text. Currently text is string and undefined type and I would say it is unlikely that it will change.

On the other hand, tooltip is still going through the accessibility sign-off review but compared to the stable one it solves multiple problems and it is more accessible. I think trying out the new tooltip in Primer rather than dotcom first seems like a good idea to me.

Posted a comment/question about the usage as well #3913 (comment) - very keen to hear what you think!

mperrotti · 2023-11-06T22:22:58Z

src/ActionList/Item.tsx

 import Box, {BoxProps} from '../Box'
+import {Tooltip} from '../drafts'


When I try and run tests locally and import the "new" Tooltip, I got this mysterious error:

FAIL src/ActionList/ActionList.test.tsx ● Test suite failed to run TypeError: Cannot read properties of undefined (reading 'LeadingVisual') 226 | export type NavListLeadingVisualProps = ActionListLeadingVisualProps 227 | > 228 | const LeadingVisual = ActionList.LeadingVisual | ^ 229 | 230 | LeadingVisual.displayName = 'NavList.LeadingVisual' 231 | at Object.LeadingVisual (src/NavList/NavList.tsx:228:34) at Object.require (src/NavList/index.ts:1:1) at Object.require (src/drafts/index.ts:51:1) at Object.require (src/ActionList/Item.tsx:18:1) at Object.require (src/ActionList/index.ts:3:1) at Object.require (src/index.ts:49:1) at Object.require (src/utils/testing.tsx:7:1) at Object.require (src/utils/test-matchers.tsx:7:1)

Any idea what might be causing this? I couldn't figure it out.

siddharthkp · 2023-11-07T10:47:21Z

src/ActionList/Item.tsx

+            {
+              // If it's inactive and isn't rendered in a container like ActionMenu or SelectPanel, render a leading visual with a tooltip
+              inactive && container === undefined ? (
+                <Tooltip text={inactiveText}>


The cursor is on the entire disabled item, should the tooltip also be on the whole item or just the icon?

inactive-item.mov

Just the icon

siddharthkp · 2023-11-07T10:50:13Z

What is the overlap with disabled state?

<ActionList.Item disabled={true}>: https://primer.style/react/storybook/?path=/story/components-actionlist-features--disabled-item

As far as I remember, disabled was added for exactly this use case. Example of disabled ActionList.Item used with Tooltip

siddharthkp · 2023-11-07T11:15:04Z

Just to confirm, I see in ActionList, we always add an alert icon. But in ActionMenu, we only add an alert icon if there is already a leading visual. Is that intentional?

ActionList:

ActionMenu:

Side note: When the other items don't have icons, the alert icon moves the position of the label. Should we try to keep the labels aligned? For example, by using trailingVisual for alert instead of leading?

mperrotti · 2023-11-07T14:52:33Z

What is the overlap with disabled state?

Disabled items don't need to meet the minimum contrast and don't need a message explaining why it's disabled. cc @alexislucio

mperrotti · 2023-11-07T14:55:03Z

Just to confirm, I see in ActionList, we always add an alert icon. But in ActionMenu, we only add an alert icon if there is already a leading visual. Is that intentional?

ActionMenu actually doesn't need to show the alert icon at all since we show the message inline. I just kept it as another visual signal that the item doesn't work. I could easily change it so that ActionMenu items never render the alert icon. What do you think?

mperrotti · 2023-11-07T14:57:32Z

When the other items don't have icons, the alert icon moves the position of the label. Should we try to keep the labels aligned? For example, by using trailingVisual for alert instead of leading?

This is such a rare state that I didn't think we need to worry about the alignment as much as we do for things like single select.

However, I like your idea about using the trailing visual slot. Let me play with it and see how it feels.

siddharthkp · 2023-11-07T15:40:53Z

Disabled items don't need to meet the minimum contrast and don't need a message explaining why it's disabled

Is there a use case for disabled items with low contrast that's not covered by inactive? 🤔

ActionMenu actually doesn't need to show the alert icon at all since we show the message inline. I just kept it as another visual signal that the item doesn't work.

Fair enough!

mperrotti · 2023-11-07T21:03:18Z

Is there a use case for disabled items with low contrast that's not covered by inactive? 🤔

If it's disabled and doesn't need to be conveyed to screen reader users. @alexislucio or somebody from a11y could probably give a more detailed answer. We have some info in the interface guidelines: https://primer.style/components/action-list#tooltips-and-dialogs-on-inactive-items

mperrotti · 2023-11-07T22:59:05Z

I took the suggestion to render the inactive indicator in the trailing visual slot to preserve the alignment. Good call @siddharthkp !

pksjce · 2023-12-09T00:10:02Z

src/ActionList/shared.ts

@@ -52,18 +58,21 @@ type MenuItemProps = {
  'aria-labelledby'?: string
  'aria-describedby'?: string
  role?: string
+  'data-inactive'?: string


I don't think this should be a prop. We set this based on inactiveText value right?

pksjce · 2023-12-09T00:10:40Z

src/ActionMenu/ActionMenu.features.stories.tsx

+  </ActionMenu>
+)
+
+// TODO: Uncomment this story when we have inactive buttons


Since inactive buttons have been merged this can be uncommented?

pksjce

Thanks for the prompt replies @mperrotti!
Left a couple of comments mainly

data-inactive does not need to be a prop

Would be great to see the following in a follow-up PR

Uncomment the story with the inactiveButton
Add more comprehensive documentation for inactiveText ie
- Its shown as tooltip on icon in ActionList and as description in ActionMenu
- It replaces LeadingVisual if one is already present and adds/replaces TrailingVisual if not. I feel this will be a bit of a surprise to consumers otherwise.
- In case of a link item, it behaves in a completely different manner.

Also look forward to the Tooltip related future bugfix that is independent of this PR.

Other than that this looks good to me. I will also close out the associated discussion with a few comments for posterity. Thanks for all your efforts!

…nlist-item

mperrotti · 2023-12-11T17:17:08Z

In case of a link item, it behaves in a completely different manner.

I'm not sure what you mean about the behavior being completely different on link items.

broccolinisoup

Tooltip usage is looking good! ✨

adds inactive state to ActionList items

103f1b1

makes unavailable message consistent fixes LinkItem styling

mperrotti requested review from a team and joshblack November 6, 2023 22:17

adds changeset

cd3c8f5

github-actions bot temporarily deployed to storybook-preview-3913 November 6, 2023 22:22 Inactive

mperrotti commented Nov 6, 2023

View reviewed changes

mperrotti temporarily deployed to github-pages November 6, 2023 22:25 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3913 November 6, 2023 22:26 Inactive

mperrotti added the update snapshots label Nov 6, 2023

mperrotti and others added 2 commits November 6, 2023 17:27

Merge branch 'main' into mp/inactive-actionlist-item

4d184d1

test(vrt): update snapshots

7f2fcfc

github-actions bot removed the update snapshots label Nov 6, 2023

github-actions bot temporarily deployed to storybook-preview-3913 November 6, 2023 22:34 Inactive

siddharthkp reviewed Nov 7, 2023

View reviewed changes

Merge branch 'main' into mp/inactive-actionlist-item

99a297e

mperrotti temporarily deployed to github-pages November 7, 2023 14:39 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3913 November 7, 2023 14:39 Inactive

render inactive indicator in trailing viz slot if no leading viz

dbb1615

mperrotti requested a review from siddharthkp November 7, 2023 22:59

github-actions bot temporarily deployed to storybook-preview-3913 December 7, 2023 18:54 Inactive

pksjce reviewed Dec 9, 2023

View reviewed changes

pksjce approved these changes Dec 9, 2023

View reviewed changes

mperrotti added 2 commits December 11, 2023 11:32

Merge branch 'main' of github.com:primer/react into mp/inactive-actio…

70f89d8

…nlist-item

rms unnecessary data-inactive prop

22e5b57

mperrotti enabled auto-merge December 11, 2023 16:42

mperrotti temporarily deployed to github-pages December 11, 2023 16:46 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3913 December 11, 2023 16:46 Inactive

mperrotti disabled auto-merge December 11, 2023 17:18

mperrotti enabled auto-merge December 11, 2023 17:18

This was referenced Dec 11, 2023

Actionlist item docs improvements #4050

Merged

Adds loading state to ActionList items #4051

Merged

Merge branch 'main' into mp/inactive-actionlist-item

e48ddc9

mperrotti temporarily deployed to github-pages December 11, 2023 18:53 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3913 December 11, 2023 18:53 Inactive

broccolinisoup approved these changes Dec 11, 2023

View reviewed changes

mperrotti added this pull request to the merge queue Dec 11, 2023

mperrotti removed this pull request from the merge queue due to a manual request Dec 11, 2023

Merge branch 'main' into mp/inactive-actionlist-item

9ef7b72

mperrotti enabled auto-merge December 11, 2023 22:12

mperrotti deployed to github-pages December 11, 2023 22:16 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-3913 December 11, 2023 22:16 View deployment

mperrotti added this pull request to the merge queue Dec 11, 2023

Merged via the queue into main with commit cf22577 Dec 11, 2023
29 checks passed

mperrotti deleted the mp/inactive-actionlist-item branch December 11, 2023 22:27

primer bot mentioned this pull request Dec 11, 2023

Release tracking #4031

Merged

primer bot mentioned this pull request Dec 19, 2023

Release tracking #4085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds inactive state to ActionList items #3913

Adds inactive state to ActionList items #3913

mperrotti commented Nov 6, 2023 •

edited

Loading

changeset-bot bot commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023

github-actions bot commented Nov 6, 2023 •

edited

Loading

mperrotti Nov 6, 2023 •

edited

Loading

mperrotti Nov 6, 2023

mperrotti Nov 6, 2023

siddharthkp Nov 7, 2023

broccolinisoup Nov 8, 2023 •

edited

Loading

broccolinisoup Nov 8, 2023

mperrotti Nov 6, 2023

siddharthkp Nov 7, 2023 •

edited

Loading

mperrotti Nov 7, 2023

siddharthkp commented Nov 7, 2023 •

edited

Loading

siddharthkp commented Nov 7, 2023 •

edited

Loading

mperrotti commented Nov 7, 2023

mperrotti commented Nov 7, 2023

mperrotti commented Nov 7, 2023

siddharthkp commented Nov 7, 2023 •

edited

Loading

mperrotti commented Nov 7, 2023

mperrotti commented Nov 7, 2023

pksjce Dec 9, 2023

pksjce Dec 9, 2023

pksjce left a comment

mperrotti commented Dec 11, 2023

broccolinisoup left a comment

		import Box, {BoxProps} from '../Box'
		import {Tooltip} from '../drafts'

Adds inactive state to ActionList items #3913

Adds inactive state to ActionList items #3913

Conversation

mperrotti commented Nov 6, 2023 • edited Loading

Screenshots

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Nov 6, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 6, 2023

github-actions bot commented Nov 6, 2023 • edited Loading

size-limit report 📦

mperrotti Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broccolinisoup Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthkp Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthkp commented Nov 7, 2023 • edited Loading

siddharthkp commented Nov 7, 2023 • edited Loading

mperrotti commented Nov 7, 2023

mperrotti commented Nov 7, 2023

mperrotti commented Nov 7, 2023

siddharthkp commented Nov 7, 2023 • edited Loading

mperrotti commented Nov 7, 2023

mperrotti commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksjce left a comment

Choose a reason for hiding this comment

mperrotti commented Dec 11, 2023

broccolinisoup left a comment

Choose a reason for hiding this comment

mperrotti commented Nov 6, 2023 •

edited

Loading

changeset-bot bot commented Nov 6, 2023 •

edited

Loading

github-actions bot commented Nov 6, 2023 •

edited

Loading

mperrotti Nov 6, 2023 •

edited

Loading

broccolinisoup Nov 8, 2023 •

edited

Loading

siddharthkp Nov 7, 2023 •

edited

Loading

siddharthkp commented Nov 7, 2023 •

edited

Loading

siddharthkp commented Nov 7, 2023 •

edited

Loading

siddharthkp commented Nov 7, 2023 •

edited

Loading